Enhance crontab job execution with re-entry protection#1317
Merged
yileicn merged 4 commits intoSciSharp:masterfrom Apr 2, 2026
Merged
Enhance crontab job execution with re-entry protection#1317yileicn merged 4 commits intoSciSharp:masterfrom
yileicn merged 4 commits intoSciSharp:masterfrom
Conversation
Contributor
Review Summary by QodoAdd distributed re-entry protection for crontab job execution
WalkthroughsDescription• Add re-entry protection mechanism to prevent concurrent crontab job execution • Introduce distributed locking using IDistributedLocker for task synchronization • Add ReentryProtection boolean flag to CrontabItem model (enabled by default) • Refactor execution logic with graceful fallback when lock acquisition fails • Improve logging with detailed lock acquisition and failure messages Diagramflowchart LR
A["CrontabItem<br/>+ ReentryProtection flag"] --> B["ExecuteTimeArrivedItemWithReentryProtection"]
B --> C{"ReentryProtection<br/>enabled?"}
C -->|Yes| D["Acquire Distributed Lock"]
C -->|No| E["Execute directly"]
D --> F{"Lock<br/>acquired?"}
F -->|Yes| G["Execute Task"]
F -->|No| H["Skip execution<br/>log warning"]
D --> I{"Exception<br/>occurred?"}
I -->|Before lock| J["Execute without lock"]
I -->|After lock| K["Log warning"]
G --> L["Release Lock"]
E --> M["Complete"]
J --> M
K --> M
File Changes1. src/Infrastructure/BotSharp.Abstraction/Crontab/ICrontabService.cs
|
Contributor
Code Review by Qodo
1. Lock key too coarse
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+140
to
+158
| var lockKey = $"crontab:execution:{item.Title}"; | ||
| using var scope = _services.CreateScope(); | ||
| var locker = scope.ServiceProvider.GetRequiredService<IDistributedLocker>(); | ||
| var acquired = false; | ||
| var lockAcquired = false; | ||
|
|
||
| try | ||
| { | ||
| acquired = await locker.LockAsync(lockKey, async () => | ||
| { | ||
| lockAcquired = true; | ||
| _logger.LogInformation("Crontab: {0}, Distributed lock acquired, beginning execution...", item.Title); | ||
| await ExecuteTimeArrivedItem(item); | ||
| }, timeout: 600); | ||
|
|
||
| if (!acquired) | ||
| { | ||
| _logger.LogWarning("Crontab: {0}, Failed to acquire distributed lock, task is still executing, skipping this occurrence to prevent re-entry.", item.Title); | ||
| } |
Contributor
There was a problem hiding this comment.
1. Lock key too coarse 🐞 Bug ≡ Correctness
ExecuteTimeArrivedItemWithReentryProtection builds the distributed lock key from only CrontabItem.Title, so different crontab items (e.g., different ConversationId/AgentId) sharing the same Title will block each other and cause skipped executions.
Agent Prompt
### Issue description
`ExecuteTimeArrivedItemWithReentryProtection` uses `crontab:execution:{item.Title}` as the distributed lock key. Since crontab items are persisted/upserted by `ConversationId` (and items also vary by `AgentId`/`UserId`), different jobs can share a title and incorrectly block each other, leading to skipped executions.
### Issue Context
- Crontab items in Mongo are upserted by `ConversationId`, not by `Title`, so `Title` is not a safe global unique identifier.
- The lock key should incorporate the job's real identity (at least `ConversationId`, and likely `AgentId` too).
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Crontab/Services/CrontabService.cs[132-158]
### Implementation notes
- Build the lock key from stable identifiers, e.g.:
- `crontab:execution:{item.TriggerType}:{item.AgentId}:{item.ConversationId}:{item.Title}`
- If some sources don’t have `ConversationId`, fall back to a safe placeholder like `global`.
- Keep the log message consistent with the new key (include key parts for debugging).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
added 2 commits
April 1, 2026 09:11
…te CrontabItemDocument to include TriggerType and ReentryProtection properties.
yileicn
reviewed
Apr 2, 2026
| public CronTabItemTriggerType TriggerType { get; set; } = CronTabItemTriggerType.BackgroundWatcher; | ||
|
|
||
| [JsonPropertyName("reentry_protection")] | ||
| public bool ReentryProtection { get; set; } = true; |
Member
|
/review |
Contributor
|
Persistent review updated to latest commit 0c67ec9 |
…iceScope; Set default value to false for perporty ReentryProtection
Member
|
reviewed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.